-
-
Notifications
You must be signed in to change notification settings - Fork 824
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
#475 - Allow changing current working directory of fd #509
Conversation
Looks good, thank you. Can we please implement a simple test for this option? |
7f1f989
to
1e81907
Compare
Sure. Pull request updated. |
Is this fine to merge it? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you very much. The code looks good.
Could you please elaborate a little bit on the interplay between this new option and --search-path
and --full-path
?
tests/tests.rs
Outdated
two/three/directory_foo", | ||
); | ||
|
||
// Ignore base directory when aboslute path is used |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo: absolute
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
src/main.rs
Outdated
let basedir = Path::new(base_directory); | ||
if !fshelper::is_dir(basedir) { | ||
print_error_and_exit!( | ||
"'{}' passed as '--base-directory' value is not a directory.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe:
The '--base-directory' path ('{}') is not a directory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. Fixed.
src/main.rs
Outdated
} | ||
if let Err(e) = env::set_current_dir(basedir) { | ||
print_error_and_exit!( | ||
"Cannot set '{}' as current working directory: {}", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor rewording:
Could not set '{}' as the current working directory: {}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
My current understanding is as follows: |
1e81907
to
2f8c466
Compare
👍
right.
makes sense.
Yes, right. Because
There is one more thing that is potentially very confusing. Consider the following (let's say we are inside the
I guess this happens because I guess we should wait with changing the directory until we have resolved other (relative) path arguments. We should also add a test for this. |
The question is whether we want to keep |
I think it should be relative to the CWD of the terminal (i.e. where |
Yeah, I get your point. Especially that docs in
Such an approach in practice means that |
Oh.. you mean because we would search the I think the following is a reasonable use case:
The I think we should also look into what other programs do. Like |
Yeap. And if path is supplied
This is the use case I had in mind when opted for making paths resolved relative to
In Git using
I still think having |
Ok, then let's do it the same way and keep the logic as is.
👍
Sounds great. Looking forward to merging this, if you could extend the documentation in that respect. |
2f8c466
to
7204c85
Compare
Great. Docs updated. Are they OK from your POV? |
Thank you very much. I might change the wording a little bit, but apart from that, it looks great. |
Thanks a lot for your help and merge. |
Hi, this PR solves #475 by implementing
--base-directory
option and usingstd::env::set_current_dir
. Feedback appreciated.